Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an example about how to use wait_for_all_acked #316

Merged

Conversation

Barry-Xu-2018
Copy link
Contributor

This commit depends on the merge of ros2/rcl#913 and ros2/rclcpp#1662.

@Barry-Xu-2018
Copy link
Contributor Author

If pre-shutdown callback is supported (ros2/rclcpp#1706 (comment)), I can simplify current codes.

@fujitatomoya
Copy link
Collaborator

probably it would be nice to have only specified example to introduce wait_for_acknowledgement w/o custom_shutdown_func? but i think it is still okay, any opinions?

@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented Jul 15, 2021

probably it would be nice to have only specified example to introduce wait_for_acknowledgement w/o custom_shutdown_func? but i think it is still okay, any opinions?

Yes.
The example want to show how to wait the confirmation of subscribers while calling shutdown.
But there is no pre-shutdown callback interface.
So I have to use custom_shutdown_func to implement it.
About 'pre-shutdown', it was discussed in ros2/rclcpp#1706 (comment).
I also submitted an implementation ros2/rclcpp#1714. But this implementation need to be discussed.
After this feature is implemented, we can simplify this example.

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 can we also have example for rclpy?

@Barry-Xu-2018
Copy link
Contributor Author

@Barry-Xu-2018 can we also have example for rclpy?

Yeah. I will consider adding this example.

@fujitatomoya
Copy link
Collaborator

we can run CI once ros2/rclcpp#1662 is merged.

@Barry-Xu-2018
Copy link
Contributor Author

Example code for python isn't added.
It is because rclpy has this issue ros2/rclpy#532. I added comments ros2/rclpy#532 (comment).

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-example-for-wait_for_all_acked branch from 7af8c38 to d00623d Compare November 30, 2021 06:07
@Barry-Xu-2018
Copy link
Contributor Author

do rebase

@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

@fujitatomoya
Copy link
Collaborator

@clalancette it appears that CI still does not have ros2/rclcpp#1662, that leads this PR to build failure. Do you know how long it takes? i was thinking this is done by nightly build.

@clalancette
Copy link
Contributor

@clalancette it appears that CI still does not have ros2/rclcpp#1662, that leads this PR to build failure. Do you know how long it takes? i was thinking this is done by nightly build.

So for the Rpr job to pass, we need to make an explicit source and bloom release to get PRs in. I can do one of those later today.

That said, you should be able to run a CI job from https://ci.ros2.org to verify this functionality on all platforms, and that will include the latest sources.

@fujitatomoya
Copy link
Collaborator

CI: (just checking if it can be build with mainline since there is no test for examples)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya

I update code to fix build warning on macOS.
Could you re-trigger CI ?

@fujitatomoya
Copy link
Collaborator

  • macOS Build Status

@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

@fujitatomoya
Copy link
Collaborator

@ivanpauno @sloretz @clalancette could you help this with review?

@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

@@ -26,6 +26,9 @@ ament_target_dependencies(publisher_member_function_with_type_adapter rclcpp std
add_executable(publisher_member_function_with_unique_network_flow_endpoints member_function_with_unique_network_flow_endpoints.cpp)
ament_target_dependencies(publisher_member_function_with_unique_network_flow_endpoints rclcpp std_msgs)

add_executable(publisher_member_function_with_wait_for_all_acked member_function_with_wait_for_all_acked.cpp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a shorter name for the example?

It really doesn't matter if this is using a "member function", I think we could rather use "publisher_wait_for_all_acked".

Comment on lines 14 to 15
#include <inttypes.h>

#include <chrono>
#include <functional>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <inttypes.h>
#include <chrono>
#include <functional>
#include <chrono>
#include <cstdint>
#include <functional>

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRId64 is included in cinttypes. So I include cinttypes instead of cstdint and adjust the order of header file as your advice.

Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-example-for-wait_for_all_acked branch from d9ea59d to ce5e34f Compare January 13, 2022 05:19
@Barry-Xu-2018
Copy link
Contributor Author

Friendly ping @ivanpauno

@ivanpauno ivanpauno merged commit 200a16c into ros2:master Jan 25, 2022
@fujitatomoya
Copy link
Collaborator

@ivanpauno really appreciate for helping on this, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants